Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SH1106 display support #846

Merged
merged 9 commits into from
Aug 28, 2023
Merged

Conversation

Anomalocaridid
Copy link
Contributor

@Anomalocaridid Anomalocaridid commented Aug 4, 2023

Currently, the OLED extension only supports displays that use the SSD1306 chipset. This PR adds support for displays with the SH1106 chipset.

Essentially, I accomplished this by making both displays display types through subclasses of a new class called OledDisplayType. These would be created and passed to the constructor for Oled rather than passing display-specific parameters such as I2C and SPI pins. Display-specific logic is handled by methods in each display's class, which are called by the Oled class's methods with the same names.

Using an SH1106 display requires the adafruit_displayio_sh1106 library, but I made it so that you only need the library for the display(s) you are using by importing them within the displays' classes' methods rather than globally.

I did some minor testing and it looks like it works fine. However, I noticed that one short line of pixels one the edge of the screen, presumably from the CircuitPython boot screen, lingers and I am not entirely sure what the cause is. Also I do not own a keyboard with a SSD1306 display, so I need someone else who has one to check that I did not break anything.

Unfortunately, this PR introduces some breaking changes for users and might interfere with #830. However, I am more than happy to wait until #830 gets merged first and then rebase my PR branch and update the documentation myself if necessary.

Instead of initializing the OLED extension with something like:

import board
import busio
from kmk.extensions.oled import Oled

i2c_bus = busio.I2C(board.SCL, board.SDA)
Oled(i2c=i2c_bus)

You would use something like:

import board
import busio
from kmk.extensions.oled import Oled, SSD1306

i2c_bus = busio.I2C(board.SCL, board.SDA)
display_type = SSD1306(i2c=i2c_bus)

oled = Oled(display_type=display)

For an SH1106 display, it would look like:

import board
import busio
from kmk.extensions.oled import Oled, SH1106

spi_bus = busio.SPI(board.SCK, board.MOSI)
display_type = SH1106(spi=spi_bus, command=board.DC, chip_select=board.CS, reset=board.RESET)

oled = Oled(display_type=display)

Note that for the SH1106 display, it requires pins other than the ones used to create spi_bus. Also, I made sure to include the option to just specify the pins and have the classes' __init__() methods create the I2C/SPI bus for you like it was before.

Copy link
Member

@regicidalplutophage regicidalplutophage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this! I was hoping oled.py could eventually become display.py, supporting other types of displays and this seems like a great blueprint for that.

I'm approving that, but I'll leave merging up to @xs5871.

@Anomalocaridid
Copy link
Contributor Author

Apparently some displays' drivers are built into CircuitPython and do not need any manual initialization. So I added support for those. Interestingly, this includes my own macropad and using this instead of the SH1106 library solves the problem I mentioned earlier:

However, I noticed that one short line of pixels one the edge of the screen, presumably from the CircuitPython boot screen, lingers and I am not entirely sure what the cause is.

You would initialize one of these displays like this:

import board
from kmk.extensions.oled import Oled, BuiltInDisplay

display_type = BuiltInDisplay(display=board.DISPLAY)

oled = Oled(display_type=display)

Of couse, using displayio.release_keymaps() would interfere with already-initialized displays, so I moved that to the other displays' __init__() methods

@xs5871
Copy link
Collaborator

xs5871 commented Aug 9, 2023

I generally approve, give some time to go into the review.

@Anomalocaridid
Copy link
Contributor Author

Sorry for adding another change after you already started reviewing my pull request, but I discovered that my last change breaks sleep and wake functionality for built-in displays, so I went ahead and fixed it.

BuiltInDisplay's constructor now takes numbers corresponding to display-specific sleep and wake commands. Sleeping and waking is now handled by the respective methods in the different display types' classes rather than directly by the display's methods.

Using a BuiltInDisplay now looks like this:

import board
from kmk.extensions.oled import Oled, BuiltInDisplay

# Command arguments shown are for example purposes. I assume different displays have different commands for this.
display_type = BuiltInDisplay(display=board.DISPLAY, sleep_command=0xAE, wake_command=0xAF)

oled = Oled(display_type=display)

Copy link
Collaborator

@xs5871 xs5871 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it has taken me so long. Here are my thoughts:

  1. We should rename the Oled module to Display. The sooner the better.
  2. This requires documentation. No docs, no merge.
  3. The OledDisplayType class has no shared code and thus doesn't add functionality but will occupy memory. It would enable static type checking, but other than that it's somewhat useless; especially since it's an internal and unlikely to be used actual users. Not a merge blocker, just my opinion. If you want it to stay, it stays.
  4. Not a big fan of the name OledDisplayType, specifically the "Type" part of the name. If at all possible, we should avoid overloading language specific nomenclature. We may want to go with DisplayBackend, DisplayDriver, DisplayInterface, ..., or something along those lines.
    4.1. Similarly, the display_type can be shortend to just display, imo.

@Anomalocaridid
Copy link
Contributor Author

Anomalocaridid commented Aug 17, 2023

How's this?

  • I changed display_type to display, replacing the existing display attribute.
  • I changed OledDisplayType to DisplayBackend. I ended up keeping it around because it gained a significant amount of shared code.
  • I changed "OLED_Display.md" to "Display.md" and updated it.
  • I changed some miscellaneous occurrences of "oled" to "display".
  • I also took the liberty of replacing the use of Display.show() with Display.root_group because it is apparently deprecated.

Copy link
Collaborator

@xs5871 xs5871 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix linter

kmk/extensions/display.py Outdated Show resolved Hide resolved
kmk/extensions/display.py Outdated Show resolved Hide resolved
@xs5871 xs5871 merged commit 61eabf5 into KMKfw:master Aug 28, 2023
1 check passed
@Anomalocaridid Anomalocaridid deleted the sh1106-display branch August 28, 2023 14:55
@Anomalocaridid
Copy link
Contributor Author

Thank you!

@regicidalplutophage
Copy link
Member

Good work @Anomalocaridid @xs5871 !

xs5871 added a commit that referenced this pull request Oct 17, 2023
* add support for sh1106 oled displays
* replace 'oled' with 'display'

---------

Co-authored-by: xs5871 <60395129+xs5871@users.noreply.github.com>
hixan pushed a commit to hixan/kmk_firmware that referenced this pull request Nov 25, 2023
* add support for sh1106 oled displays
* replace 'oled' with 'display'

---------

Co-authored-by: xs5871 <60395129+xs5871@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants